Skip to content

feat(skill): bcli skill init — interactive wizard mechanism (AIP Phase 7)#19

Merged
igor-ctrl merged 1 commit into
mainfrom
feat/skill-init-mechanism
May 17, 2026
Merged

feat(skill): bcli skill init — interactive wizard mechanism (AIP Phase 7)#19
igor-ctrl merged 1 commit into
mainfrom
feat/skill-init-mechanism

Conversation

@igor-ctrl
Copy link
Copy Markdown
Owner

@igor-ctrl igor-ctrl commented May 17, 2026

Summary

Phase 7 mechanism of AIP v0.1. New bcli skill init / bcli skill update wizard that consumes bcli describe and generates a per-user Claude Code skill bundle plus optional new saved-query entries.

Mechanism only. OSS bcli ships no Beautech-specific role templates. Third-party packages (bcli-beautech-bootstrap per Worker C's track) plug in via a new bcli.skill_init.role_templates entry-point group. OSS default proposer returns an empty list — opinion-free.

Flow

Per contract doc §Phase 8:

  1. Subprocess bcli describe --format json (honors --profile).
  2. Four Rich prompts: role, top-3 daily questions, slash-command style, whether to generate new queries.
  3. Project existing saved queries — top-3 free text fuzzy-matches against query description + endpoint via difflib (stdlib). No role-keyed BC-entity affinities live in OSS — that would be Beautech-flavored content masquerading as a generic mechanism.
  4. If the operator opted into proposals, run every registered role-template provider and gate each suggestion behind a per-query [y/N] Confirm.
  5. Stage the SKILL.md + queries YAML changes, run them through the allow-list guardrail, commit atomically.

Hard guarantees (each pinned by a test)

Guarantee Test
Reads describe via subprocess; doesn't import CLI internals TestReadDescribe::test_wizard_loads_describe_payload
Writes ONLY under ~/.config/bcli/queries/, ~/.claude/skills/bcli-<user>/, and wizard bookkeeping dir TestGuardrails::*
New saved-query proposals require explicit [y/N] per query TestApprovalGate::*
Provenance frontmatter on every generated file TestProvenance::test_skill_md_carries_provenance_frontmatter
Existing saved queries are never modified TestProvenance::test_appended_query_carries_inline_provenance
Wizard never writes to config.toml, registries, or disable_* flags TestReadonlyConsumption::*
Atomic rollback: partial-write failure leaves no files TestAtomicRollback::test_partial_write_failure_leaves_no_files
Idempotent: update --non-interactive replays the cached interview AND previously-approved queries TestIdempotency::test_update_replays_previously_approved_queries (key invariant — see PR section below)
Describe payload hash mismatch refuses silent replay TestIdempotency::test_update_reinterviews_when_describe_version_changes
Default proposer is empty for every role (no BC affinities in OSS) TestOssMechanismHasNoRoleContent::*

Why caching the approved-queries list matters

The v1 design only cached InterviewState — not the operator's per-query y/N answers. A skill update --non-interactive would then silently drop every approved new query from SKILL.md on every replay, breaking the documented idempotency contract.

Fixed by serializing both the interview AND the approved query bodies into ~/.config/bcli/skills/.last-init.json. Replay re-applies the same writes verbatim. Test test_update_replays_previously_approved_queries pins it.

Extension point for downstream packages

# In bcli-beautech-bootstrap's pyproject.toml:
[project.entry-points."bcli.skill_init.role_templates"]
beautech = "bcli_beautech_bootstrap.skill_templates:propose"

propose(interview, payload) -> list[ProposedQuery] returns role-keyed proposals (aviation_ops, finance_close, etc.). The wizard discovers it via importlib.metadata.entry_points — no hard-coded knowledge of which downstream packages exist. Worker C builds this on the bootstrap side.

Output files

  • ~/.config/bcli/queries/<profile>.yaml — existing entries untouched; approved new entries appended with a per-query provenance block.
  • ~/.claude/skills/bcli-<user>/SKILL.md — atomic-write; YAML frontmatter with generated-by, version, profile, role, generated_at, source_hash.
  • ~/.config/bcli/skills/.last-init.json — wizard bookkeeping for non-interactive replay.

<user> is getpass.getuser() (the OS username — matches how Claude Code skills are scoped on disk).

Test plan

  • uv run pytest tests/ -v — 819 passed, 5 skipped (was 818; +20 new minus 1 — full suite tally is +1 since one earlier idempotency test was strengthened to also check SKILL.md content).
  • uv run ruff check src/ tests/ — clean.
  • CLI smoke: bcli skill --help shows init and update subcommands with the expected flags.
  • Reviewer: run bcli skill init interactively against a real profile and confirm the generated SKILL.md is well-formed and the queries YAML is unchanged for entries that pre-existed.

Known limitations

  1. --ai-author agent integration not wired. Contract doc §Phase 8 describes spawning a local Claude/Codex subagent to author per-user descriptions. Today's wizard is fully deterministic — the entry-point group is the agent-integration surface for v0.2. Documented inline; not in v0.1 spec.
  2. <user> ambiguity. Resolved via getpass.getuser(). Multi-user installs (rare for bcli) all share the same skill dir per OS user; documented in the source.
  3. Worker A's feat/skill-install not yet merged. When it lands, both PRs add a skill Typer group — the merge will trivially fold both subcommands into one group (each PR uses add_typer(skill_*.app, name="skill")). Self-resolves on merge order.

Spec / context

@igor-ctrl
feat(skill): bcli skill init — interactive wizard mechanism (AIP Phas…
6258aa3 
…e 7)

New ``bcli skill init`` / ``bcli skill update`` wizard that consumes
``bcli describe`` and generates a per-user Claude Code skill bundle
plus optional new saved-query entries. **Mechanism only** — the OSS
package ships no Beautech-specific role templates; third-party
packages (``bcli-beautech-bootstrap``) plug in via the
``bcli.skill_init.role_templates`` entry-point group.

Wizard flow matches contract doc §Phase 8:

1. Subprocess ``bcli describe --format json`` (uses the optional
   ``--profile`` override).
2. Four Rich prompts: role, top-3, slash-command style, whether to
   generate new queries.
3. Project existing saved queries — top-3 free text fuzzy-matches
   against query descriptions + endpoints via ``difflib`` (stdlib).
   No role-keyed BC-entity affinities live in OSS.
4. If the user opted into proposals, run every registered role-
   template provider and gate each suggestion behind a per-query
   ``[y/N]`` Confirm prompt.
5. Compose the SKILL.md + queries YAML changes, run them through
   the allow-list guardrail, then commit atomically.

Hard guarantees (each pinned by a test under ``tests/test_skill_init/``):

* Reads ``bcli describe`` via subprocess; never imports the CLI's
  internals. Works against any bcli version emitting the AIP §Phase 1
  schema.
* Writes ONLY under ``~/.config/bcli/queries/``,
  ``~/.claude/skills/bcli-<user>/``, and the wizard's bookkeeping
  dir at ``~/.config/bcli/skills/``. Any other destination raises
  :class:`SkillInitError` before the file is opened.
* New saved-query proposals require explicit ``[y/N]`` per query.
* Provenance frontmatter (YAML) on every generated file.
* Existing saved queries are NEVER modified — new queries are
  appended with an inline ``provenance`` block so a later
  ``skill update`` can find its own work.
* Atomic rollback: all writes are staged, then committed in order;
  any failure restores pre-commit content (or deletes files that
  didn't exist before).
* Idempotent: the interview state AND the approved query bodies are
  cached at ``~/.config/bcli/skills/.last-init.json``.
  ``bcli skill update --non-interactive`` replays the same writes
  verbatim — including any queries the operator previously approved
  (test ``test_update_replays_previously_approved_queries`` pins
  this; without it, replay silently drops approved queries from
  SKILL.md, which is the invariant the v1 wizard would have shipped
  with had I not surfaced the issue with the advisor).
* A describe-payload hash mismatch refuses the silent replay and
  asks the operator to re-run interactively.

Tests: 20 new under ``tests/test_skill_init/``. Suite total 819
passed (was 818); ruff clean. No new runtime deps — ``rich`` is
already in ``[cli]`` and ``difflib`` / ``importlib.metadata`` /
``getpass`` are stdlib.

Beautech-specific role templates ship separately in
``bcli-beautech-bootstrap`` (per Worker C's track) via the
entry-point group above.
igor-ctrl
igor-ctrl commented May 17, 2026
View reviewed changes
Copy link
Copy Markdown
Owner Author

@igor-ctrl igor-ctrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lead review — APPROVE (pending Igor's merge)

Reviewed against agent-cli-contract-v0.1.md §Phase 8 and the seven review foci. Worker B self-report verified: 819 passed / 5 skipped, ruff clean, MERGEABLE / CLEAN, CI all green on 3.11/3.12/3.13. The advisor-caught replay-correctness fix is exactly the right thing, and the OSS / Beautech split is clean.

Seven foci → findings

1. Opinion-free OSS / entry-point design. ✓

  • _ENTRYPOINT_GROUP = "bcli.skill_init.role_templates" discovered via importlib.metadata.entry_points(group=...).
  • _default_role_template_proposer returns [] unconditionally — pinned by test_default_proposer_empty_for_finance_role and test_default_proposer_empty_for_any_role across ops/aviation/sales/dev/custom.
  • Per-provider exceptions caught and logged; one broken plugin can't break the wizard.
  • Provider callable signature documented in the _default_role_template_proposer docstring: Callable[[InterviewState, dict[str, Any]], list[ProposedQuery]].

2. Snapshot-rollback commit phase. ✓ with a minor coverage observation

  • _commit_plan builds the in-memory plan → pre-flights _assert_writable on every target before any file opens → snapshots existing content (or None) → _atomic_write per target via tmp + os.replace → on exception, restores snapshots (write old text back, or unlink if file didn't exist).
  • State cache is best-effort (failure here doesn't trigger rollback because no user-facing artifact changed). Documented in source.
  • test_partial_write_failure_leaves_no_files pins the contract (queries YAML untouched, no SKILL.md created on commit failure).
  • Coverage observation (non-blocking): the test monkeypatches _commit_plan to raise immediately, so the "first write committed, second failed, rollback first" inner loop (lines 608-617) is not directly exercised. The contract-that-matters (no partial state) is empirically pinned, but the rollback inner loop could use a tighter test. Track for v0.5.1.
  • No file lock / refuse-if-running for concurrent invocations. Acceptable for a single-user wizard but worth noting if someone scripts it.

3. Idempotent replay correctness fix. ✓

  • state_cache_text persists both interview and approved_new_queries (each with name + body). Non-interactive branch reconstructs ProposedQuery instances from cache["approved_new_queries"].
  • test_update_replays_previously_approved_queries exercises the full round-trip: interactive init approves 2 proposals → interview_answers.clear() → non_interactive=True → plan2.approved_new_queries == plan1.approved_new_queries AND queries YAML retains both entries.
  • Minor: the PR body claims "schema is forward-compatible (version field; tolerates old cache files on read)" — there's no explicit version field in the cache JSON. In practice the describe payload_hash check serves the same purpose: bcli's tool_version feeds into the hash, so a cache-schema-changing upgrade also invalidates cache. The end-user contract holds, but the PR body wording overstates. Not blocking — but please correct the PR body before merge, or follow up with an explicit cache_version field in v0.5.1 so the wizard doesn't rely on describe-payload version drift to invalidate its own format.

4. Describe-hash mismatch + manual edit. ✓

  • test_update_reinterviews_when_describe_version_changes mutates payload version to "0.2", runs --non-interactive, expects SkillInitError(match=r"changed"). Hash mismatch correctly refuses silent replay.
  • Manual edits preserved: _stage_queries_file_update reads the current YAML at commit time, merges with approved new queries via dict(existing) then raw["queries"] = queries. Existing entries pass through untouched (asserted by test_appended_query_carries_inline_provenance).

5. Guardrails enforcement. ✓

  • Four explicit guardrail tests: refuses ~/.bashrc, accepts queries dir, accepts bcli-* skill dir, rejects other skill dirs (only bcli-* subdirs allowed under ~/.claude/skills/).
  • Two integration tests run the full wizard with a sentinel file under ~/.config/bcli/config.toml and ~/.config/bcli/registries/ — both untouched.
  • _assert_writable uses Path.resolve(strict=False) followed by is_relative_to(...) — symlinks/relative paths resolved first, then containment checked. Canonical pattern.
  • Provenance frontmatter on SKILL.md (YAML between --- markers); per-query inline provenance: block on appended saved queries.

6. Rich prompt mocking. ✓

  • interview_answers fixture monkeypatches rich.prompt.Prompt.ask + rich.prompt.Confirm.ask with a sequential (needle, answer) lookup.
  • 4 questions pinned in order: Role → daily → style → Generate. A refactor that reordered questions OR changed prompt wording would break tests as intended.

7. Coordination with Worker A. ⚠ Merge-time concern (flag for the team)

app.py line 190 registers skill_init_cmd.app as the skill Typer group:

app.add_typer(skill_init_cmd.app, name="skill", help="Generate a per-user bcli skill bundle (AIP Phase 7)")

Worker A's Phase 6 (bcli skill install) is reportedly using a separate skill_cmd.py. If Worker A also calls app.add_typer(skill_cmd.app, name="skill", ...), the two registrations collide on the skill name. Typer doesn't compose two Typer instances under the same name — the second add_typer either errors or overwrites.

Clean integration path for Worker A's PR:

  • Option (a) — Worker A's skill_cmd.py imports skill_init_cmd.app and registers install on it: @skill_init_cmd.app.command("install") def install_command(...).
  • Option (b) — rename skill_init_cmd.app to a shared skill_cmd.app in this PR (or worker A's), and have both init/update and install register on it.

Recommend (a): minimal cross-PR churn; Worker A imports the module that already exists rather than restructuring. Either way, the merge of Worker A's PR with main will need attention.

Additional notes (non-blocking)

  • update_command is currently init_command(...) verbatim. The two commands stay separate for help-string + future-evolution reasons (documented), but the implementation today is identical. Acceptable for v0.1 — flag for v0.5.x if a future update-specific behavior emerges (e.g., describe-version diff diagnostic).
  • generated_at ticks forward on --non-interactive replay — SKILL.md frontmatter mtime/content hash changes every run. The idempotency test compares the body below the frontmatter close, which is the right contract. Downstream content-hash watchers would see noise; documented in the test's docstring.
  • Provider integration surface (InterviewState, ProposedQuery, Callable[[InterviewState, dict[str, Any]], list[ProposedQuery]]) is documented in docstrings only. Worker C's bootstrap repo will need to import from bcli_cli.commands.skill_init_cmd. A future v0.5.x could promote the protocol to a public bcli.skill_init namespace to reduce the coupling.

Verification summary

  • Local full suite: 819 passed, 5 skipped, ruff clean in 22.45s
  • Focused: 20 skill_init tests all pass
  • CI: SUCCESS on 3.11 / 3.12 / 3.13
  • Mergeable: CLEAN
  • Stdlib only (getpass, hashlib, difflib, importlib.metadata); rich + yaml + typer already in [cli] extra. No new runtime deps.

Release-notes items for 0.5.0 (Phase 6 + 7 train)

Captured for when 0.5.0 release plan gets drafted after Worker A's Phase 6 + Worker C's bootstrap PR also land:

  • New bcli skill init / bcli skill update commands (interactive wizard, atomic commit, idempotent replay).
  • New entry-point group bcli.skill_init.role_templates for downstream Beautech-style providers.
  • Wizard's allow-list: writes only under ~/.config/bcli/queries/, ~/.claude/skills/bcli-<user>/, ~/.config/bcli/skills/. Refuses everywhere else.
  • Provenance frontmatter on generated SKILL.md; per-query inline provenance: on appended saved queries.

Verdict: APPROVE. Igor merges; I do not.

Worker A's Phase 6 PR — please flag the merge coordination on the skill Typer group registration before the next push.

@igor-ctrl igor-ctrl mentioned this pull request May 17, 2026
Merged
6 tasks
@igor-ctrl igor-ctrl merged commit 3d1e8da into main May 17, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@igor-ctrl